Can one of the admins verify this patch?

does not support:

• Parquet-mr reader due to lack of field id support (needs a follow up ticket)

Just for my own knowledge: what needs to be done to make parquet-mr support field id?

does not support:

• Parquet-mr reader due to lack of field id support (needs a follow up ticket)

Just for my own knowledge: what needs to be done to make parquet-mr support field id?

I am still investigating, previously I thought it requires support from parquet-mr, but now looks like it's not necessary.

I am working on a fix locally, which might be pushed out as part of this PR or another.

@jackierwzhang
FYI: I am working with @shangxinli on column id resolution in parquet-mr link, with pretty much the same motivation as yours. The work will probably overlap with yours.
One thing that I just realized is that the field id can be NOT unique in schema. For example:

message ParquetSchema {
  required group reqMap (MAP) = 1 {
    repeated group key_value (MAP_KEY_VALUE) {
      required binary key (STRING);
      optional group value (MAP) {
        repeated group key_value (MAP_KEY_VALUE) {
          required binary key (STRING);
          optional group value {
            required binary name (STRING) = 1;
            optional binary age (STRING) = 2;
            optional binary gender (STRING) = 3;
            optional group addedStruct = 4 {
              required binary name (STRING) = 1;
              optional binary age (STRING) = 2;
              optional binary gender (STRING) = 3;
            }
          }
        }
      }
    }
  }
}

I probably need to change the format specification to make the field id unique.

@huaxingao

Got it.

As for duplicated field id, I think in my approach, reading parquet files with duplicated id across different groups are allowed, essentially we just don't want confusion when matching fields which are on the same level in the schema.

Btw just curious, since you have been working on field id resolution for parquet-mr, do you know whether it currently supports reading and writing field ids yet?

I think in my approach, reading parquet files with duplicated id across different groups are allowed, essentially we just don't want confusion when matching fields which are on the same level in the schema.

Sounds reasonable. I hope I can do the same too, but seems to me that I need to resolve the column by id only, which requires that the id to be unique in the entire schema. This is going to be a breaking change. Not sure if I am allowed to do it or not.

It doesn't seem to me that parquet-mr supports reading and writing field ids yet. The field ids are not in ColumnDescriptor.

@huaxingao Are you suggesting the code that:

1. sets field id like https://github.com/apache/parquet-mr/blob/1adc22804a700d78f8480667d083e91d6147339f/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L279, and
2. reads field id like https://github.com/apache/parquet-mr/blob/5608695f5777de1eb0899d9075ec9411cfdf31d3/parquet-thrift/src/main/java/org/apache/parquet/thrift/ThriftSchemaConvertVisitor.java#L271

aren't really doing anything?

@jackierwzhang
No, those are set correctly.
What I meant is that the field ids are not really used. Seems only the ColumnPath is used in column index, column resolution, etc. I am thinking of adding field id in ColumnDescriptor and keeping a map between id and ColumnDescriptor, or a map between id and ColumnPath.

@jackierwzhang No, those are set correctly. What I meant is that the field ids are not really used. Seems only the ColumnPath is used in column index, column resolution, etc. I am thinking of adding field id in ColumnDescriptor and keeping a map between id and ColumnDescriptor, or a map between id and ColumnPath.

Got it. I was asking because I tested locally and found that parquet-mr can actually save and read field ids via Spark, so I don't have to patch anything for the parquet-mr repo.

Tho there are a couple of small problems remaining for id matching on the parquet-mr side, I believe It's possible to extend this PR (or open another) to enable spark to match by id in that code path; I'm gonna do that soon.

thanks, merging to master!